client impl part 1 #112
client impl part 1 #112
Conversation
|
Claude finished @randygrok's task —— View job Code Review AnalysisI've completed a comprehensive review of this client implementation for EvNode transactions. Here's my detailed feedback:
Overall AssessmentThis is a well-structured implementation of a Viem client for custom EvNode transactions. The code demonstrates good TypeScript practices, comprehensive RLP encoding/decoding, and thoughtful API design. However, there are several important issues that should be addressed. Critical Issues🔴 Security:
|
Summary of ChangesHello @randygrok, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes the foundational client-side logic for a new EvNode transaction system. It defines the structure and behavior of these specialized transactions, enabling developers to programmatically construct, sign with multiple parties (executor and sponsor), and dispatch them to the network. The implementation focuses on robust data handling, signature management, and integration with existing blockchain interaction libraries to streamline the development of applications utilizing this new transaction type. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Viem client for a custom EvNode transaction type. The implementation is comprehensive, covering transaction encoding/decoding, signing, and client-side actions. My review focuses on improving robustness, correctness of gas estimations, and maintainability. I've identified a few areas for improvement, including more accurate gas calculations, safer error handling in RPC calls, and addressing potential issues with the eth_sign method. I've also suggested some minor refactoring to reduce code duplication.
clients/src/index.ts
Outdated
| export function hashSignerFromRpcClient( | ||
| client: Client, | ||
| address: Address, | ||
| ): HashSigner { | ||
| return { | ||
| address, | ||
| signHash: async (hash) => { | ||
| // eth_sign is expected to sign raw bytes (no EIP-191 prefix). | ||
| const signature = await client.request({ | ||
| method: 'eth_sign', | ||
| params: [address, hash], | ||
| }); | ||
| if (!isHex(signature)) { | ||
| throw new Error('eth_sign returned non-hex signature'); | ||
| } | ||
| return signature; | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The eth_sign RPC method has inconsistent behavior across different Ethereum node implementations and is generally considered deprecated. Some nodes (like Geth) will sign the raw hash as expected here, but others (like Parity/OpenEthereum) will prefix the hash according to EIP-191, which would lead to invalid signatures for this use case. This can cause hard-to-debug issues for users of your client depending on which RPC provider they use.
It's highly recommended to add a prominent warning in the function's documentation about this limitation and the specific node behavior it relies on.
clients/src/index.ts
Outdated
| async function fetchMaxPriorityFee(client: Client): Promise<bigint> { | ||
| try { | ||
| const result = await client.request({ method: 'eth_maxPriorityFeePerGas' }); | ||
| if (!isHex(result)) throw new Error('eth_maxPriorityFeePerGas returned non-hex'); | ||
| return hexToBigIntSafe(result); | ||
| } catch { | ||
| return 0n; | ||
| } | ||
| } |
There was a problem hiding this comment.
The broad catch block here will swallow any error from the eth_maxPriorityFeePerGas call and default to 0n. This can hide underlying problems like network connectivity issues or RPC misconfigurations, leading to transactions being sent with a potentially undesirable maxPriorityFeePerGas. It's safer to only catch specific errors that indicate the method is not supported (like on a pre-EIP-1559 chain) and re-throw others.
try {
const result = await client.request({ method: 'eth_maxPriorityFeePerGas' });
if (!isHex(result)) throw new Error('eth_maxPriorityFeePerGas returned non-hex');
return hexToBigIntSafe(result);
} catch (err) {
// Only fallback to 0n if the method is not supported.
// A robust implementation would check for a specific RPC error code (e.g., -32601).
if (err instanceof Error && (err.message.includes('not found') || err.message.includes('not supported'))) {
return 0n;
}
throw err;
}
clients/src/index.ts
Outdated
| export function estimateIntrinsicGas(calls: Call[]): bigint { | ||
| let gas = 21000n; | ||
|
|
||
| for (const call of calls) { | ||
| if (call.to === null) gas += 32000n; | ||
|
|
||
| for (const byte of hexToBytes(call.data)) { | ||
| gas += byte === 0 ? 4n : 16n; | ||
| } | ||
| } | ||
|
|
||
| return gas; | ||
| } |
There was a problem hiding this comment.
The current implementation of estimateIntrinsicGas does not account for the gas costs associated with an accessList (per EIP-2930). This will lead to an underestimation of the intrinsic gas for transactions using access lists, potentially causing them to fail. The function should be updated to accept an accessList and include its cost in the calculation.
export function estimateIntrinsicGas(calls: Call[], accessList?: AccessList): bigint {
let gas = 21000n;
for (const call of calls) {
if (call.to === null) gas += 32000n;
for (const byte of hexToBytes(call.data)) {
gas += byte === 0 ? 4n : 16n;
}
}
if (accessList) {
const ACCESS_LIST_ADDRESS_COST = 2400n;
const ACCESS_LIST_STORAGE_KEY_COST = 1900n;
gas += BigInt(accessList.length) * ACCESS_LIST_ADDRESS_COST;
for (const item of accessList) {
gas += BigInt(item.storageKeys.length) * ACCESS_LIST_STORAGE_KEY_COST;
}
}
return gas;
}
clients/src/index.ts
Outdated
| async sendEvNodeTransaction(args: { | ||
| calls: Call[]; | ||
| executor: HashSigner; | ||
| chainId?: bigint; | ||
| nonce?: bigint; | ||
| maxFeePerGas?: bigint; | ||
| maxPriorityFeePerGas?: bigint; | ||
| gasLimit?: bigint; | ||
| accessList?: AccessList; | ||
| }): Promise<Hex> { |
There was a problem hiding this comment.
The args parameter uses an inline anonymous type that is very similar to the EvnodeSendArgs interface defined earlier. This creates code duplication and can make maintenance harder.
To improve this, consider defining a specific type for these arguments that extends EvnodeSendArgs but makes executor a required property. This would make the code more DRY and easier to reason about.
clients/src/index.ts
Outdated
| async createSponsorableIntent(args: { | ||
| calls: Call[]; | ||
| executor: HashSigner; | ||
| chainId?: bigint; | ||
| nonce?: bigint; | ||
| maxFeePerGas?: bigint; | ||
| maxPriorityFeePerGas?: bigint; | ||
| gasLimit?: bigint; | ||
| accessList?: AccessList; | ||
| }): Promise<SponsorableIntent> { |
There was a problem hiding this comment.
Similar to sendEvNodeTransaction, this function uses an inline anonymous type for its args parameter which duplicates most of EvnodeIntentArgs. Reusing and extending the existing interface would make the code more maintainable and reduce redundancy.
clients/src/index.ts
Outdated
| const nonce = overrides.nonce ?? (await fetchNonce(client, address)); | ||
| const maxPriorityFeePerGas = | ||
| overrides.maxPriorityFeePerGas ?? (await fetchMaxPriorityFee(client)); | ||
| const maxFeePerGas = overrides.maxFeePerGas ?? (await fetchGasPrice(client)); |
There was a problem hiding this comment.
Defaulting maxFeePerGas to the result of eth_gasPrice can be suboptimal for EIP-1559 transactions, as it might lead to overpaying for gas. A more conventional approach is to calculate maxFeePerGas as baseFeePerGas + maxPriorityFeePerGas. This would require fetching the baseFeePerGas from the latest block.
clients/src/index.ts
Outdated
| const maxPriorityFeePerGas = | ||
| overrides.maxPriorityFeePerGas ?? (await fetchMaxPriorityFee(client)); | ||
| const maxFeePerGas = overrides.maxFeePerGas ?? (await fetchGasPrice(client)); | ||
| const gasLimit = overrides.gasLimit ?? estimateIntrinsicGas(calls); |
There was a problem hiding this comment.
To fix the underestimation of intrinsic gas, you should pass the accessList to estimateIntrinsicGas here, assuming you've updated the function as per my other comment.
const gasLimit = overrides.gasLimit ?? estimateIntrinsicGas(calls, accessList);- Add 21000 gas per call in estimateIntrinsicGas for batch support - Unify env vars to use EXECUTOR_PRIVATE_KEY across all tests - Rewrite evnode-flows.ts with balance verification for all 4 flows
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
Nice over all looking good!
A few things and then I think we can merge if the E2Es are passing
- can we add some unit tests and CI for the client (first set can be minimal I think, something we can add to)
- update the package.json to allow for running the
basic/flows/sponsoredtests. - Add a basic linter or formatter in the package.json also (and run in CI)
|
merging to main, next step will be to checkout this into the impl branch |
3 participants
Description
Closes: #109